Skip to content

use onMouseDown instead of setTimeout in FocusManager #103

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 13, 2018
Merged

use onMouseDown instead of setTimeout in FocusManager #103

merged 1 commit into from
May 13, 2018

Conversation

@souporserious
Copy link
Collaborator

@souporserious souporserious commented May 11, 2018 •
edited
Loading

Came across some bugs recently at work with the setTimeout method and animations 😓, but it led me down a better path so I wanted to share it here 😇. Stoked to get rid of the hacky setTimeout so now everything should just work as normal 🎉.

TrySound
TrySound approved these changes May 12, 2018
View reviewed changes
Copy link
Collaborator

@TrySound TrySound left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, Travis. I like this.

@TrySound
Copy link
Collaborator

TrySound commented May 12, 2018 •
edited
Loading

By the way in which browsers did you test it?

@souporserious
Copy link
Collaborator Author

souporserious commented May 12, 2018

Thank you! I tested all major browsers on macOS as well as mobile Safari and Chrome on iOS and even IE browsers through browserstack and all seems well!

@souporserious
Copy link
Collaborator Author

souporserious commented May 12, 2018 •
edited
Loading

Just had a thought... do you think it would be helpful to add a prop like blurOnEscape? I know that this is usually a necessity for components for things like popovers, menus, etc. where this component will probably be used. That being said, we may want a focusLastElementOnBlur prop as well, since that has to do with proper accessibility 🤷‍♂️ not sure if that's getting too out of scope though.

@TrySound
Copy link
Collaborator

TrySound commented May 12, 2018

I'm not sure. This will break single responsibility of this component. However achieving this using .blur command is tricky because we need a couple of lifecycles inside. And still I guess there will be a lifecycle component in react itself so this problem may be fixed soon. Let's just leave it without change and consider later, maybe open an issue about that.

@souporserious
Copy link
Collaborator Author

souporserious commented May 12, 2018

Sounds good!

@TrySound TrySound merged commit 833c3b9 into master May 13, 2018
@TrySound TrySound deleted the better-focus-manager-component branch May 13, 2018 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrySound TrySound TrySound approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@souporserious @TrySound